-
Notifications
You must be signed in to change notification settings - Fork 360
Allow for time-numoffset with and without colon separator #838
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…using draft-06 schema
@matason Thanks for the contribution. I've done some checking and the test case that is removed here has been part of the library for the past 12 years. I noticed that in the Draft06 branch the RFC3339 class was altered so I believe the regression occurs somewhere in that class and the fix should be applied there as well. Since the test case has been part of the codebase for the past 12 years there is a highly likely chance it is being used somewhere. Would you mind taking another look and see if you can update to PR to fix the issue in the RFC3339 class? |
@DannyvdSluijs that’s a really good point, that that has been in there for so long is a good indicator that removing it is not the right thing to do. I’ll absolutely have a look at the class and see what I can figure out. Thanks for the kind reply. |
52a9019
to
34009c5
Compare
Hi there, I've put back the 2000-05-01T12:12:12+0100 test case in the data provider and made a small change to the regex to allow for an offset with and without the colon separator. I'm still wondering whether an offset without a colon is valid - https://datatracker.ietf.org/doc/html/rfc3339#section-5.6 has On the current draft-06-support branch, this commit reduces the test failures from 15 to 13. |
673037d
to
8dff21a
Compare
Thanks for the fix. I've cherry picked fc994a7 into #835 You're correct that offset without a colon is invalid. However this has become part of the expected behaviour of existing implementations so I would rather not break those. I've added it to the list of deprecations in the next mayor version. With the commit cherry picked this PR can be closed. Once again thanks for the work done 👍🏼 |
closes #837